Upgrade ios packages and update contract#338
Conversation
Co-authored-by: Dishit <hanmadishit74@gmail.com>
Co-authored-by: Dishit <hanmadishit74@gmail.com>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request introduces several new fields—modelKey, modelType, combinedTotalBytes, and metadataJson—to the iOS DownloadManagerModule to mirror Android's WorkManager columns and ensure consistent JS hydration across platforms. Additionally, the llama-rn dependency is updated, and the TypeScript background download service is adjusted to support the new downloadId property. Feedback was provided regarding an inconsistent default value for modelType in the DownloadManagerModule extension, which should be aligned with other parts of the module to maintain a consistent data contract.
| status: "running", | ||
| startedAt: Date().timeIntervalSince1970 * 1000, | ||
| modelKey: params["modelKey"] as? String, | ||
| modelType: (params["modelType"] as? String) ?? "image", |
There was a problem hiding this comment.
The default value for modelType is set to "image" here, which is inconsistent with startDownload (line 558), the restoration logic (line 499), and the decoding logic (line 158), all of which default to "text". To ensure a consistent data contract across the module and with the JavaScript service, this should default to "text".
| modelType: (params["modelType"] as? String) ?? "image", | |
| modelType: (params["modelType"] as? String) ?? "text", |
There was a problem hiding this comment.
Fixed — changed default to "text" to be consistent with startDownload, the persistence decoder, and restoreTasksFromSession. JS always passes modelType explicitly for multifile downloads so the default never fired in practice, but the inconsistency has been resolved.
There was a problem hiding this comment.
Code Review
This pull request introduces several new fields (modelKey, modelType, combinedTotalBytes, and metadataJson) to the iOS DownloadManagerModule to ensure cross-platform consistency with Android and improve JavaScript hydration. It also updates the llama-rn dependency and adjusts the background download service to support the new downloadId property. Feedback was provided regarding potential state loss during task restoration, suggesting these fields be stored in TaskDescription for better recovery. Additionally, an inconsistency in the default value for modelType across different initialization paths was identified.
| modelKey: nil, | ||
| modelType: "text", | ||
| combinedTotalBytes: 0, | ||
| metadataJson: nil, |
There was a problem hiding this comment.
These new fields are being initialized with hardcoded defaults during task restoration. If the app's persisted state in UserDefaults is lost or out of sync, but background URLSessionTasks are still active, the restored DownloadInfo will lose its original modelKey, modelType, combinedTotalBytes, and metadataJson. To ensure robustness and avoid swallowing the state-loss issue with defaults, these fields should be added to the TaskDescription struct so they can be recovered directly from the URLSessionDownloadTask.taskDescription property.
References
- Instead of swallowing errors by only logging them, functions that can fail should propagate the result (e.g., by returning a success/failure status or throwing an error) to allow callers to handle it.
There was a problem hiding this comment.
Acknowledged. Adding these fields to TaskDescription is the more robust solution for the edge case where URLSession tasks survive but UserDefaults is wiped. In practice these always travel together on iOS — if UserDefaults is cleared the session tasks are invalidated too. We've accepted this as a known limitation given the added complexity of expanding TaskDescription encoding. The decodeIfPresent fallbacks protect the common upgrade path.
| status: "running", | ||
| startedAt: Date().timeIntervalSince1970 * 1000, | ||
| modelKey: params["modelKey"] as? String, | ||
| modelType: (params["modelType"] as? String) ?? "image", |
There was a problem hiding this comment.
The default value for modelType here is "image", whereas in startDownload (line 558) and restoreTasksFromSession (line 499) it defaults to "text". This inconsistency can lead to unexpected behavior. Consider using a consistent default or ensuring the type is explicitly passed from the JavaScript side.
| modelType: (params["modelType"] as? String) ?? "image", | |
| modelType: (params["modelType"] as? String) ?? "text", |
There was a problem hiding this comment.
Fixed — changed default to "text" to match startDownload and the rest of the module.
There was a problem hiding this comment.
Code Review
This pull request introduces several new fields (modelKey, modelType, combinedTotalBytes, and metadataJson) to the iOS DownloadManagerModule to align with the Android implementation and support cross-platform JS hydration. It also updates the llama-rn dependency and adjusts the TypeScript service to handle the new downloadId field. Feedback identified a potential race condition in download ID generation, inconsistent default values for modelType across different methods, and a naming mismatch for the creation timestamp compared to the Android and TypeScript implementations.
| let downloadId = String(nextDownloadId) | ||
| nextDownloadId += 1 |
There was a problem hiding this comment.
The nextDownloadId is being accessed and incremented outside of the synchronized queue. Since this module uses a concurrent queue for state management and can be called from multiple threads via the React Native bridge, this creates a race condition that could lead to duplicate downloadId values.
| let downloadId = String(nextDownloadId) | |
| nextDownloadId += 1 | |
| let downloadId = queue.sync(flags: .barrier) { | |
| let id = String(nextDownloadId) | |
| nextDownloadId += 1 | |
| return id | |
| } |
There was a problem hiding this comment.
Fixed — nextDownloadId read and increment are now wrapped in queue.sync(flags: .barrier) matching the rest of the module's state management pattern.
| "startedAt": NSNumber(value: info.startedAt), | ||
| "modelType": info.modelType, | ||
| "combinedTotalBytes": NSNumber(value: info.combinedTotalBytes) |
There was a problem hiding this comment.
The iOS implementation returns the timestamp as startedAt, but the TypeScript service (line 90) and the Android implementation use createdAt. To ensure cross-platform compatibility and correct hydration in the JS layer, it is recommended to include the createdAt key in the response.
"startedAt": NSNumber(value: info.startedAt),
"createdAt": NSNumber(value: info.startedAt),
"modelType": info.modelType,
"combinedTotalBytes": NSNumber(value: info.combinedTotalBytes)There was a problem hiding this comment.
Fixed — getActiveDownloads now includes a createdAt key aliased from startedAt so the JS mapper receives the correct field name on both platforms.
| status: "running", | ||
| startedAt: Date().timeIntervalSince1970 * 1000, | ||
| modelKey: params["modelKey"] as? String, | ||
| modelType: (params["modelType"] as? String) ?? "image", |
There was a problem hiding this comment.
The default modelType here is set to "image", which is inconsistent with startDownload (line 558), the persistence decoder (line 158), and the session restoration logic (line 499), all of which default to "text". This inconsistency can lead to models being incorrectly categorized depending on the download method used or after an app restart.
| modelType: (params["modelType"] as? String) ?? "image", | |
| modelType: (params["modelType"] as? String) ?? "text", |
There was a problem hiding this comment.
Fixed — changed default to "text" for consistency across the module.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
- Coverage 82.32% 82.31% -0.02%
==========================================
Files 231 231
Lines 11900 11901 +1
Branches 3265 3266 +1
==========================================
- Hits 9797 9796 -1
Misses 1246 1246
- Partials 857 859 +2
🚀 New features to boost your workflow:
|
…odelType default, add createdAt
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the DownloadManagerModule in iOS to include new fields (modelKey, modelType, combinedTotalBytes, and metadataJson) for better alignment with Android's WorkManager and improved JS hydration. It also introduces thread-safe generation of downloadId using a barrier queue. Additionally, the llama-rn dependency is updated in Podfile.lock, and the TypeScript backgroundDownloadService is adjusted to handle the updated download ID field. I have no feedback to provide.



Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes